⚡ perf: Optimize deleteBulk N+1 issue with single IN query#82
Conversation
Optimizes `deleteBulk` in `SqlDelightBlockRepository` to use a single `IN` query to fetch block data instead of iterating and doing N+1 separate lookups. Adds `selectBlocksByUuids` query to `SteleDatabase.sq` and modifies `deleteBulk` to chunk the `IN` query inputs by 900 elements to prevent SQLite `too many variables` limits. Benchmark testing using `jdbc:sqlite::memory:` shows ~7x performance improvement (from ~521 ms to ~70 ms) for deleting 1000 blocks. Co-authored-by: tstapler <3860386+tstapler@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
This PR optimizes deleteBulk in the SQLDelight block repository by adding a bulk block lookup query and using chunked IN queries before deletion.
Changes:
- Adds
selectBlocksByUuidsto fetch multiple blocks by UUID. - Updates
deleteBulkto skip empty input and prefetch requested blocks in chunks. - Replaces per-UUID initial block lookup with a UUID-to-block map.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
kmp/src/commonMain/sqldelight/dev/stapler/stelekit/db/SteleDatabase.sq |
Adds the bulk UUID lookup SQL query. |
kmp/src/commonMain/kotlin/dev/stapler/stelekit/repository/SqlDelightBlockRepository.kt |
Uses the new query in deleteBulk and adds an empty-list fast path. |
Comments suppressed due to low confidence (2)
kmp/src/commonMain/kotlin/dev/stapler/stelekit/repository/SqlDelightBlockRepository.kt:380
- Prefetching all blocks before the transaction loop makes each block's
left_uuidstale after an earlier deletion repairs the sibling chain. For adjacent UUIDs like[B, C], deletingBupdatesC.left_uuid, but processing prefetchedCstill uses the oldleft_uuidand can update the following sibling to point at the deleted block (or hit a foreign-key error). The loop needs to refresh mutable linkage fields before each deletion or otherwise compute repairs from current DB state.
val blocksByUuid = blocks.associateBy { it.uuid }
blockUuids.forEach { uuid ->
val block = blocksByUuid[uuid] ?: return@forEach
kmp/src/commonMain/kotlin/dev/stapler/stelekit/repository/SqlDelightBlockRepository.kt:376
- This bulk fetch only loads the UUIDs passed into
deleteBulk; whendeleteChildrenis true (the default), subtree collection below still issues oneselectBlockChildrenquery per deleted descendant. Bulk deleting a large subtree therefore still has an N+1 query pattern despite this newINquery, so the optimization does not cover the common recursive-delete path.
val blocks = blockUuids.chunked(900).flatMap { chunk ->
queries.selectBlocksByUuids(chunk).executeAsList()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val blocks = blockUuids.chunked(900).flatMap { chunk -> | ||
| queries.selectBlocksByUuids(chunk).executeAsList() | ||
| } |
JVM Load Benchmark (Desktop)Synthetic in-memory benchmark measuring load performance for the desktop (JVM) app.
Flamegraphs (this PR)**Allocation** — object allocation pressure (JDBC/SQLite churn)Alloc flamegraph not available CPU — method-level hotspots by on-CPU time CPU flamegraph not available Top SQL queries by total time (this PR)| table:operation | calls | p50 | p99 | max | total | |-----------------|-------|-----|-----|-----|-------| | `pages:select` | 2 | 1ms | 1ms | 1ms | 2ms |Top allocation hotspots (this PR)`66.7%` byte[]_[k] `4.8%` java.lang.String_[k] `4.3%` java.lang.Object[]_[k] `3.2%` java.lang.StringBuilder_[k] `2.2%` java.lang.Class_[k]Top CPU hotspots (this PR)`99.5%` /usr/lib/x86_64-linux-gnu/libc.so.6 `0%` Dictionary::find_class `0%` /tmp/sqlite-3.51.3.0-c4142376-45cb-4adf-934f-306b7045c8cc-libsqlitejdbc.so `0%` java/lang/Throwable.fillInStackTrace_[1] `0%` sem_post |
Android Load BenchmarkInstrumented benchmark on an API 30 x86_64 emulator — 500-page synthetic graph. Comparing Graph Load
Interactive Write Latency (during Phase 3)
SAF I/O Overhead (ContentProvider vs direct File read)Measures Binder IPC cost added by ContentResolver per readFile() call.
|
💡 What:
selectBlocksByUuidsquery toSteleDatabase.sqto fetch blocks by a list of UUIDs.deleteBulkinSqlDelightBlockRepositoryto perform a chunked (size=900) bulk fetch of all blocks that need to be deleted before iterating through them.blockUuidsis non-empty before starting the db transaction.🎯 Why:
Previously,
deleteBulkfetched every block sequentially inside a loop (queries.selectBlockByUuid(uuid).executeAsOneOrNull()), resulting in the N+1 query problem. This bulk delete is significantly faster and uses less CPU/memory footprint when the user tries to delete large numbers of blocks.📊 Measured Improvement:
PR created automatically by Jules for task 2324316106947837264 started by @tstapler